feat(cmdk): Create dynamic command palette action registration#111495
feat(cmdk): Create dynamic command palette action registration#111495
Conversation
github-actions
bot
added
the
Scope: Frontend
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| return results | ||
| .flatMap(section => section.hits) | ||
| .slice(0, MAX_RESULTS) | ||
| .map(hit => | ||
| makeCommandPaletteCallback({ | ||
| display: { | ||
| label: stripHtml(hit.title ?? ''), | ||
| labelNode: highlightMarks(hit.title ?? ''), | ||
| details: hit.context?.context1, |
There was a problem hiding this comment.
Bug: Actions generated from doc search results with identical titles will have the same key, causing one to silently overwrite the other in the command palette.
Severity: MEDIUM
Suggested Fix
Update the key generation logic to ensure uniqueness. Include more information in the key, such as the documentation section or a unique identifier from the search result hit object, to prevent collisions. For example: const actionKey = ${id}:${action.type}:${hit.section}:${slugify(action.display.label)}`;.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: static/app/components/commandPalette/useDocsSearchActions.tsx#L63-L71
Potential issue: The key for command palette actions is generated by slugifying the
action's label. In `useDocsSearchActions.tsx`, actions are created from documentation
search results. If two search results from different sections (e.g., 'docs' and
'develop') have the same title, they will produce identical keys after slugification.
The reducer in `context.tsx` handles key collisions by overwriting the existing action
with the new one. This causes one of the search results to be silently dropped from the
command palette, making it inaccessible to the user without any warning.
Did we get this right? 👍 / 👎 to inform future reviews.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const MARK_PATTERN = new RegExp(`(${MARK_OPEN}.*?${MARK_CLOSE})`, 'g'); | ||
|
|
||
| function stripHtml(html: string): string { | ||
| return html.replace(/<[^>]*>/g, ''); |
Check failure
Code scanning / CodeQL
Incomplete multi-character sanitization High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 22 days ago
General approach: replace the ad‑hoc regex-based HTML stripper with a more robust, well-tested sanitization strategy that cannot reintroduce <script or other tags via multi-character replacement quirks. Since this is a React/TypeScript frontend, a small, focused HTML sanitizer library is appropriate and avoids implementing brittle custom logic.
Best concrete fix here: import a lightweight sanitizer such as dompurify and use it to produce safe, plain text from the Algolia result fields. Specifically:
- Add an import for DOMPurify at the top of
useDocsSearchActions.tsx. - Rewrite
stripHtmlso that it:- Sanitizes the input HTML with DOMPurify.
- Extracts text content without any tags. For a robust solution that doesn’t depend on browser DOM APIs (which can be problematic in SSR), DOMPurify provides
DOMPurify.sanitize(html, {ALLOWED_TAGS: [], ALLOWED_ATTR: []})to remove all tags, leaving only text.
- Ensure
highlightMarkscontinues to operate on the original Algolia-marked HTML string, but when it produces non-highlighted text fragments, it uses our newstripHtmlimplementation, which is now robust.
Concretely:
- In
static/app/components/commandPalette/useDocsSearchActions.tsx, modifystripHtmlto callDOMPurify.sanitize(html, {ALLOWED_TAGS: [], ALLOWED_ATTR: []})instead of usinghtml.replace(/<[^>]*>/g, ''). - Add an import at the top:
import DOMPurify from 'dompurify';
This keeps all existing behavior (returning plain text for labels and non-highlighted fragments) but uses a correct, repeatable, and library-backed sanitization that won’t suffer from incomplete multi-character replacement issues.
| @@ -1,5 +1,6 @@ | ||
| import {Fragment, useCallback, useState} from 'react'; | ||
| import {SentryGlobalSearch} from '@sentry-internal/global-search'; | ||
| import DOMPurify from 'dompurify'; | ||
|
|
||
| import {makeCommandPaletteCallback} from 'sentry/components/commandPalette/makeCommandPaletteAction'; | ||
| import type {CommandPaletteAction} from 'sentry/components/commandPalette/types'; | ||
| @@ -14,7 +15,8 @@ | ||
| const MARK_PATTERN = new RegExp(`(${MARK_OPEN}.*?${MARK_CLOSE})`, 'g'); | ||
|
|
||
| function stripHtml(html: string): string { | ||
| return html.replace(/<[^>]*>/g, ''); | ||
| // Use DOMPurify to remove all HTML tags and attributes, leaving only text content. | ||
| return DOMPurify.sanitize(html, {ALLOWED_TAGS: [], ALLOWED_ATTR: []}); | ||
| } | ||
|
|
||
| /** |
rbro112
changed the title
|
This issue has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you remove the label "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
2 participants
Creates dynamic actions registration following the approach outlined by https://linear.app/getsentry/issue/DE-719/cmdk-add-api-for-registering-dynamic-actions
Adds the first usage of this dynamic acton with a docs search.
